Skip to content

Conversation

@reinhold-willcox
Copy link
Collaborator

@reinhold-willcox reinhold-willcox commented Nov 27, 2024

Fixes to the vector3d function that were resulting in anomalous component velocities.

The fixes pertain to

  • Incorrect indexing. For vec = {x, y, z}, vec[i] was retrieving the value at {x, y, z}[i - 1], cyclically.
  • Incorrect rotation. The rotation algebra was wrong so the magnitude of the vector under rotation was doubling.

The bugs were introduced in June, so any results between then and now that depend on any vector quantities (or their magnitudes) should be double checked / rerun.

Copy link
Collaborator

@jeffriley jeffriley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @reinhold-willcox . Those changes were me - my fault. Again, this highlights the need for better regression testing...

@jeffriley jeffriley merged commit 7d7b913 into dev Nov 27, 2024
2 checks passed
@jeffriley jeffriley deleted the sn_kick_fixes branch November 27, 2024 18:52
@jeffriley
Copy link
Collaborator

Actually, this code:

// Operator overloads
double   operator [] (const size_t p_i) const { return (*const_cast<Vector3d*>(this))[p_i]; }
double&  operator [] (const size_t p_i) {

    THROW_ERROR_IF(p_i < 0 || p_i > 2, ERROR::INDEX_OUT_OF_RANGE);                                  // this is a code defect

         if (p_i == 1) return m_x;
    else if (p_i == 2) return m_y;
    else               return m_z;
}

was this in the original code:

double&  operator [] (const size_t p_i) {

    // check that supplied index is in range.
    // if index is not in range, issue a warning and use index modulo 3
    size_t index = p_i;
    if (index > 2) {
        SHOW_WARN(ERROR::INDEX_OUT_OF_RANGE, "Using index modulo 3");                               // index out of range
        index %= 3;                                                                                 // use index modulo 3
    }
    
         if (index == 1) return m_x;
    else if (index == 2) return m_y;
    else                 return m_z;
}

so the checks:

         if (index == 1) return m_x;
    else if (index == 2) return m_y;

were there in the original code. I think changing them to 0 and 1 is correct - but the problem has been around longer than v03.00.00

@jeffriley
Copy link
Collaborator

Ok, looks like the problem with the index checking was introduced in v02.43.04 (by me...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Companion ejection velocity seems to be a factor of 2 too large

3 participants